First implementation of PDF viewer feature #138

mneuwert · 2018-10-29T21:19:47Z

Description

PDF viewer shall be implemented allowing comfortable viewing of PDF files.

Related Issue

#87

Motivation and Context

We would like to utilise PDFKit features to provide user experience similar to iBooks app while browsing PDF files.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

PDF viewer with thumbnail view which can be hidden
Go to page X feature
Search feature showing the hits with page numbers and allowing to jump to selected search hit
Separate PDF navigation screen with two tabs: table of contents / larger thumbnails for all pages

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

CLAassistant · 2018-10-29T21:19:54Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ mneuwert
❌ felix-schwarz
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codecov · 2018-10-29T21:33:00Z

Codecov Report

Merging #138 into master will increase coverage by <.01%.
The diff coverage is 0.26%.

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   24.83%   24.84%   +<.01%     
==========================================
  Files          88       88              
  Lines        7493     7442      -51     
==========================================
- Hits         1861     1849      -12     
+ Misses       5632     5593      -39

Impacted Files	Coverage Δ
ownCloud/Viewer/PDFViewerViewController.swift	`0% <0%> (ø)`	⬆️
ownCloud/Viewer/PDFTocTableViewController.swift	`0% <0%> (ø)`	⬆️
ownCloud/Viewer/PDFOutlineViewController.swift	`0% <0%> (ø)`	⬆️
ownCloud/Viewer/PDFTocTableViewCell.swift	`0% <0%> (ø)`	⬆️
ownCloud/Viewer/PDFSearchTableViewCell.swift	`0% <0%> (ø)`	⬆️
ownCloud/UIKit Extensions/String+Extension.swift	`40% <0%> (-60%)`	⬇️
ownCloud/Viewer/PDFSearchViewController.swift	`0% <0%> (ø)`	⬆️
ownCloud/Viewer/PDFTocItem.swift	`0% <0%> (ø)`	⬆️
...nCloud/Viewer/PDFThumbnailCollectionViewCell.swift	`0% <0%> (ø)`	⬆️
...Viewer/PDFThumbnailsCollectionViewController.swift	`0% <0%> (ø)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce6ad46...1445b36. Read the comment docs.

- Fixed swiftlint warnings - Added missing removal of block-based notification observer and made the block hold self as weak reference, so it can't cause a retain cycle

pablocarmu · 2018-10-30T09:51:22Z

ownCloud/Viewer/PDFSearchTableViewCell.swift

+        self.contentView.addSubview(pageLabel)
+
+        titleLabel.font = UIFont.systemFont(ofSize: titleFontSize, weight: UIFont.Weight.regular)
+        titleLabel.adjustsFontSizeToFitWidth = false


The default value is already set to false. see adjustFontSizeToFitWidth

@pablocarmu yes, this line can be removed. The intention was to make the code more explicit

@pablocarmu fixed

pablocarmu · 2018-10-30T10:09:33Z

ownCloud/Viewer/PDFThumbnailsCollectionViewController.swift

+                return
+            }
+            DispatchQueue.main.async {
+                if let cell : PDFThumbnailCollectionViewCell = self.collectionView?.cellForItem(at: indexPath) as? PDFThumbnailCollectionViewCell {


I've seen this in multiple places and it's only my preference of styling things, but you can chain if let and avoid a los of { }

Something like:

if let cell : PDFThumbnailCollectionViewCell = self.collectionView?.cellForItem(at: indexPath) as? PDFThumbnailCollectionViewCell, let visibleCells = self.collectionView?.visibleCells, visibleCells.contains(cell) { cell.imageView?.image = thumbnailImage cell.pageLabel?.text = page.label }

@pablocarmu yes, true. I can change it.

@pablocarmu I checked once again, here chaining would sacrifice readability and in fact I only found one single other piece of code where chaining could have been applied.

pablocarmu · 2018-10-30T10:12:21Z

ownCloud/Viewer/PDFViewerViewController.swift

+    fileprivate let ANIMATION_DUR = 0.25
+    fileprivate let THUMBNAIL_VIEW_WIDTH_MULTIPLIER: CGFloat = 0.15
+    fileprivate let THUMBNAIL_VIEW_HEIGHT_MULTIPLIER: CGFloat = 0.1
+    fileprivate let FILENAME_CONTAINER_TOP_MARGIN: CGFloat = 10.0


I've seen that in some places you code constants in different styles, is there some reason to do it?

@pablocarmu for me it was not clear if we have any official coding conventions and I didn't find an example of constants in existing code. What is the preferred style?

Currently, we don't have any rule defined for this neither any complete style guide apart of what we have in the SwiftLint file.

I think we can discuss the style guide in the new opened issue here #139

@pablocarmu fixed

felix-schwarz · 2018-10-30T11:42:49Z

I found a couple of glitches worth fixing before diving into code review:

the name of the file is currently shown in the navigation bar and in an overlay. Having it in the navigation bar should be sufficient:

Tapping on the table of contents, a white background shows when dragging the table view down or up beyond its upper and lower end (rubberband-style):

The background has the correct color after switching to the the icon view and back.

When I search for something and tap on a hit, I see the search interface dismissing twice.
While entering something in search, the separator lines are moving around unexpectedly after every keystroke.
tapping on the page dismisses the navigation bar, but leads to a jumpy animation for both the page view and the thumbnails.
The page is cut off at the top and bottom when zooming in:

If I zoom into a page and then tap it, the zoom resets while going in/out of full screen mode. IIRC zoom mostly only resets after a double tap in other apps.

mneuwert · 2018-10-31T15:07:09Z

@felix-schwarz Regarding your comment:

the name of the file is currently shown in the navigation bar and in an overlay. Having it in the navigation bar should be sufficient:

I was thinking that we might add more icons in the navigation bar and therefore introduced an overlay but forgot to remove the file name from navigation bar.

@fschwarz

Which fixes glitch nr. 2 mentioned by @fschwarz

To avoid visual glitch

- Theming support - Thumbnail layout

…tion # Conflicts: # ownCloud.xcodeproj/project.pbxproj

felix-schwarz

Thanks for fixing the visual glitches and kudos to a really solid user experience! 👍

I made a few comments but from my perspective, this could also get merged as-is and the issues raised be addressed in a feature/pdfRefinements branch.

With regards to the icons, it'd be good to keep track of who made them and how they are licensed. Did you draw them yourself in the scope of this project, or use third-party ones?

ownCloud/Viewer/PDFViewerViewController.swift

ownCloud/Viewer/PDFSearchViewController.swift

felix-schwarz · 2018-11-16T08:54:05Z

ownCloud/Viewer/PDFViewerViewController.swift

+
+        guard let currentPageLabel = pdfView.currentPage?.label else { return }
+
+        pageCountLabel.text = "\(currentPageLabel) of \(pdf.pageCount)"


ownCloud Manual.pdf has "wild" page labels, so I get to see:

"1 of 79" on the first page

"i of 79" on the third page

"1 of 79" on the fifth page

"75 of 79" on the last page

I'd like to suggest:

using PDFDocument.index(for: page) for this label

using PDFPage.label only in the table of contents

localizing the string

@felix-schwarz good point!

@felix-schwarz however.. I think there is a problem with your suggestion to use PDFDocument.index(for: page) since it returns 0-based index.. so if page numbers start with something like i, ii, iii, 1, this method would return value 3 for page labeled "1".

@mneuwert Good point! The first page should be page 1 of course :) … since the y element of the "x of y" text is using the page count, the x should be the page offset (+1) instead of the label to stay consistent.

@felix-schwarz There is still an issue, see ownCloud Manual.pdf: sequence of page label in this document is:
1, 2, i, ii, 1, 2, 3 etc

@mneuwert I think for this "[page] of [total]" box at the bottom, PDF page labels are meaningless and prone to be inconsistent and confusing. Let's go for PDFDocument.index(for: page) + 1.

ownCloud/Resources/Assets.xcassets/ic_pdf_view_continuous.imageset/Contents.json

felix-schwarz · 2018-11-16T12:32:10Z

ownCloud/Viewer/PDFViewerViewController.swift

+    fileprivate let pdfView = PDFView()
+    fileprivate let thumbnailView = PDFThumbnailView()
+
+    fileprivate let containerView = UIStackView()


Using a UIStackView and putting the PDFView into it will make it hard/impossible to show PDF content "underneath" elements above and below the pdf view like in Apple's Books.

Switching to a UIView for the containerView would make that possible and generally give more room to the content.

ownCloud/Viewer/PDFOutlineViewController.swift

ownCloud/Viewer/PDFTocTableViewCell.swift

ownCloud/Viewer/PDFThumbnailsCollectionViewController.swift

felix-schwarz · 2018-11-16T13:26:40Z

ownCloud/Viewer/PDFThumbnailCollectionViewCell.swift

+        self.contentView.addSubview(imageView!)
+
+        pageLabel = UILabel()
+        pageLabel?.backgroundColor = UIColor.clear


Page labels are difficult to read if the page is darker. As far as I can see, Apple Books uses a white (or bright gray?) color here so the page number remains legible even on dark backdrops. Getting that "label" look of Apple Books, though, may require another container view around the label.

mneuwert · 2018-11-16T13:42:05Z

@felix-schwarz Regarding icons, I used Google Material icons in PDF which use Apache v2 style license if I remember correctly.

Link to Material icons which also references license:
https://google.github.io/material-design-icons/
http://www.apache.org/licenses/LICENSE-2.0.txt

And the search UI is being dismissed, when user taps “Cancel”

felix-schwarz

Thanks for making the changes. The only thing I'd still like to see changed is using the page offset (+1) instead of the page label, which should be a very minor change. The rest is better dealt with in a refinement branch.

Approved! :-)

…page

Not using page label but rather index of the current page + 1

jesmrec · 2018-11-19T12:22:08Z

Very good job @mneuwert!!. I have noticed a couple of things:

In searching mode, the separator appears only with a small set of matches. When you type only a letter (f.ex, an a), the matches are shown with no separator. When you type a more accurate input and returns a smaller set of matches, separator is displayed.
Regarding this @felix-schwarz comment:

If I zoom into a page and then tap it, the zoom resets while going in/out of full screen mode. IIRC zoom mostly only resets after a double tap in other apps.

The only way to reestablish original page size after zooming in/out is repainting (change orientation or page up/down). Double tap should do that

mneuwert · 2018-11-19T13:21:52Z

@jesmrec Fixed an issue with separator. Looks like zooming with double-tap is broken, when PDFView is configured to use UIPageViewController.

jesmrec · 2018-11-20T11:00:36Z

Approved

First implementation of PDF viewer feature

3106879

mneuwert added the p2-high Escalation, on top of current planning, release blocker label Oct 29, 2018

mneuwert added this to the 0.1.0 milestone Oct 29, 2018

mneuwert mentioned this pull request Oct 29, 2018

Feature/pdf viewer #118

Closed

10 tasks

Adjusting thumbnail size leads to crashes in certain scenarios

d12243a

jesmrec requested review from pablocarmu and jesmrec October 30, 2018 08:12

- Added license texts

212fc96

- Fixed swiftlint warnings - Added missing removal of block-based notification observer and made the block hold self as weak reference, so it can't cause a retain cycle

jesmrec requested a review from felix-schwarz October 30, 2018 10:06

pablocarmu suggested changes Oct 30, 2018

View reviewed changes

michaelstingl modified the milestones: 0.1.0, 0.2.0 Oct 30, 2018

mneuwert added 2 commits October 31, 2018 15:55

Code styling related changes

70f31d4

Code review related changes affecting styling / readability

d5fcd8a

mneuwert added 9 commits October 31, 2018 16:32

Implemented proper theming support in PDF table of contents

123b19e

Which fixes glitch nr. 2 mentioned by @fschwarz

Disabled hiding of navigation bar

ba780a8

To avoid visual glitch

Showing file name of PDF document only in the navigation bar

7afef4b

Tweaked thumbnail size

f109a16

Fixed an exception related to setting of thumbnail size

ddd95de

PDF search UI fixes

7ef9db3

Fixes in the full screen thumbnails view

acd14fa

- Theming support - Thumbnail layout

Merge remote-tracking branch 'origin/master' into feature/pdf_integra…

1b9dd3e

…tion # Conflicts: # ownCloud.xcodeproj/project.pbxproj

Removed not used code

4c80dc8

felix-schwarz requested changes Nov 16, 2018

View reviewed changes

Removed unused icons from asset catalog

6bd7aa1

mneuwert added 5 commits November 16, 2018 15:16

Improved PDF search UX (search field is focused immediately)

e9cf02b

And the search UI is being dismissed, when user taps “Cancel”

Fixed pontential crash in asynchronous thumbnail generation code

851808b

Fixed theming support in PDF outline / thumbnail UI

9aa68f9

Fixed SwiftLint warning (trailing whitespace)

d0fb58c

Improved current page number display in PDF viewer

a4856f5

felix-schwarz approved these changes Nov 16, 2018

View reviewed changes

mneuwert added 2 commits November 16, 2018 16:26

Showing an error if user tries to “go to” invalid / non-existing PDF …

3bcceba

…page

Changed the way current page nr is displayed

e181904

Not using page label but rather index of the current page + 1

Fixed table view separator glitches in PDF search

1445b36

jesmrec approved these changes Nov 20, 2018

View reviewed changes

jesmrec added the Approved by QA Approved by QA label Nov 20, 2018

jesmrec merged commit e5518a1 into master Nov 20, 2018

jesmrec deleted the feature/pdf_integration branch November 20, 2018 11:43

jesmrec mentioned this pull request Jan 22, 2019

Roadmap #94

Open

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First implementation of PDF viewer feature #138

First implementation of PDF viewer feature #138

mneuwert commented Oct 29, 2018 •

edited

Loading

CLAassistant commented Oct 29, 2018 •

edited

Loading

codecov bot commented Oct 29, 2018 •

edited

Loading

pablocarmu Oct 30, 2018

mneuwert Oct 30, 2018

mneuwert Oct 31, 2018

pablocarmu Oct 30, 2018

mneuwert Oct 30, 2018

mneuwert Oct 31, 2018

pablocarmu Oct 30, 2018

mneuwert Oct 30, 2018

pablocarmu Oct 30, 2018

mneuwert Oct 31, 2018

felix-schwarz commented Oct 30, 2018

mneuwert commented Oct 31, 2018

felix-schwarz left a comment

felix-schwarz Nov 16, 2018

mneuwert Nov 16, 2018

mneuwert Nov 16, 2018

felix-schwarz Nov 16, 2018

mneuwert Nov 16, 2018

felix-schwarz Nov 16, 2018 •

edited

Loading

felix-schwarz Nov 16, 2018

felix-schwarz Nov 16, 2018

mneuwert commented Nov 16, 2018 •

edited

Loading

felix-schwarz left a comment

jesmrec commented Nov 19, 2018

mneuwert commented Nov 19, 2018 •

edited

Loading

jesmrec commented Nov 20, 2018


		guard let currentPageLabel = pdfView.currentPage?.label else { return }

		pageCountLabel.text = "\(currentPageLabel) of \(pdf.pageCount)"

First implementation of PDF viewer feature #138

First implementation of PDF viewer feature #138

Conversation

mneuwert commented Oct 29, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

CLAassistant commented Oct 29, 2018 • edited Loading

codecov bot commented Oct 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix-schwarz commented Oct 30, 2018

mneuwert commented Oct 31, 2018

felix-schwarz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix-schwarz Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mneuwert commented Nov 16, 2018 • edited Loading

felix-schwarz left a comment

Choose a reason for hiding this comment

jesmrec commented Nov 19, 2018

mneuwert commented Nov 19, 2018 • edited Loading

jesmrec commented Nov 20, 2018

mneuwert commented Oct 29, 2018 •

edited

Loading

CLAassistant commented Oct 29, 2018 •

edited

Loading

codecov bot commented Oct 29, 2018 •

edited

Loading

felix-schwarz Nov 16, 2018 •

edited

Loading

mneuwert commented Nov 16, 2018 •

edited

Loading

mneuwert commented Nov 19, 2018 •

edited

Loading